Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ui/ffi: add timestamp to reaction group senders #2153

Merged
merged 11 commits into from
Jul 10, 2023
Merged

ui/ffi: add timestamp to reaction group senders #2153

merged 11 commits into from
Jul 10, 2023

Conversation

aringenbach
Copy link
Contributor

Expose the timestamp for each sender on a reaction group. Fixes #1792

@aringenbach aringenbach requested a review from a team as a code owner June 26, 2023 14:42
@aringenbach aringenbach requested review from poljar and removed request for a team June 26, 2023 14:42
@aringenbach aringenbach changed the title Add timestamp to reaction group senders ui/ffi: Add timestamp to reaction group senders Jun 26, 2023
@aringenbach aringenbach changed the title ui/ffi: Add timestamp to reaction group senders ui/ffi: add timestamp to reaction group senders Jun 26, 2023
@codecov
Copy link

codecov bot commented Jun 26, 2023 •
edited
Loading

Codecov Report

Patch coverage: 86.76% and project coverage change: +0.06 🎉

Comparison is base (4e52125) 76.78% compared to head (75cd565) 76.85%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2153      +/-   ##
==========================================
+ Coverage   76.78%   76.85%   +0.06%     
==========================================
  Files         169      169              
  Lines       17822    17852      +30     
==========================================
+ Hits        13685    13720      +35     
+ Misses       4137     4132       -5
Impacted Files Coverage Δ
...rates/matrix-sdk-ui/src/timeline/event_item/mod.rs 64.42% <ø> (ø)
crates/matrix-sdk-ui/src/timeline/event_handler.rs 76.71% <79.31%> (+2.51%) ⬆️
crates/matrix-sdk-ui/src/timeline/inner.rs 73.33% <90.90%> (+0.16%) ⬆️
...s/matrix-sdk-ui/src/timeline/event_item/content.rs 62.25% <100.00%> (+0.37%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@aringenbach aringenbach force-pushed the aringenbach/add-timestamp-to_reaction-group branch from 9f9be2c to d8d2b07 Compare June 27, 2023 09:40
@aringenbach aringenbach marked this pull request as draft June 27, 2023 09:41
@aringenbach aringenbach mentioned this pull request Jun 28, 2023
Closed
@aringenbach aringenbach marked this pull request as ready for review June 28, 2023 12:30
@aringenbach aringenbach requested a review from jplatte June 28, 2023 12:53
@aringenbach aringenbach mentioned this pull request Jun 28, 2023
Closed
Hywan
Hywan reviewed Jun 29, 2023
View reviewed changes
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch doesn't explain why we need to add a timestamp to reaction. Apart from that, the code looks generally good. Tests for timestamps are missing: yes timestamps are created, but they aren't tested on their own. Can you please add these :-)?
I would also rebase the commit history a little bit if possible.

crates/matrix-sdk-ui/src/timeline/event_item/mod.rs Show resolved Hide resolved
crates/matrix-sdk-ui/src/timeline/event_item/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-ui/src/timeline/event_item/mod.rs Outdated Show resolved Hide resolved
@aringenbach aringenbach force-pushed the aringenbach/add-timestamp-to_reaction-group branch from f71a0a2 to caafaaa Compare July 3, 2023 14:55
@aringenbach aringenbach requested a review from Hywan July 3, 2023 16:12
Hywan
Hywan approved these changes Jul 5, 2023
View reviewed changes
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks.

jplatte
jplatte approved these changes Jul 5, 2023
View reviewed changes
Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Ready for merge, but let's wait a bit (until our current release blocker PR is merged).

@aringenbach
Copy link
Contributor Author

@jplatte Before merging this, there is an issue around timestamps for reactions.
When loading a room, reactions often have a timestamp that is equal to now() instead of the actual value. This seems like it is a synapse / sliding sync issue, but perhaps it makes sense to wait on this change integration, until we figure it out and fix it ?

@aringenbach aringenbach force-pushed the aringenbach/add-timestamp-to_reaction-group branch from b9bc5e8 to 38a60e1 Compare July 6, 2023 16:31
@aringenbach aringenbach requested review from jplatte and Hywan July 7, 2023 07:18
@aringenbach
Copy link
Contributor Author

I think this should be ready to go, the issue came from this code actually, it was because when we create the initial timeline items we put aside reactions that are step upon before the message they relate to and resolve these later. These were not stored with their timestamp in my initial implementation.

Fix commit is 1cee900
I also added a test covering that issue just in case: 6c835d0

aringenbach and others added 2 commits July 10, 2023 11:37
@aringenbach
Merge branch 'main' into aringenbach/add-timestamp-to_reaction-group
7bb12e2 
# Conflicts:
#	crates/matrix-sdk-ui/src/timeline/inner.rs
#	crates/matrix-sdk-ui/src/timeline/tests/mod.rs
@aringenbach
Update crates/matrix-sdk-ui/src/timeline/tests/reactions.rs
75cd565
@aringenbach aringenbach force-pushed the aringenbach/add-timestamp-to_reaction-group branch from c73be28 to 75cd565 Compare July 10, 2023 09:39
@jplatte jplatte enabled auto-merge (squash) July 10, 2023 09:49
@jplatte jplatte merged commit 89aa291 into main Jul 10, 2023
@jplatte jplatte deleted the aringenbach/add-timestamp-to_reaction-group branch July 10, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonnyandrew jonnyandrew jonnyandrew left review comments

@poljar poljar Awaiting requested review from poljar poljar is a code owner automatically assigned from matrix-org/rust

@jplatte jplatte Awaiting requested review from jplatte

@Hywan Hywan Awaiting requested review from Hywan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose more data in EventTimeline's Reactions
4 participants
@aringenbach @Hywan @jplatte @jonnyandrew